Skip to content

repokitteh: adding warning about draft PRS#17063

Merged
alyssawilk merged 4 commits intoenvoyproxy:mainfrom
alyssawilk:draft
Jun 23, 2021
Merged

repokitteh: adding warning about draft PRS#17063
alyssawilk merged 4 commits intoenvoyproxy:mainfrom
alyssawilk:draft

Conversation

@alyssawilk
Copy link
Contributor

Hey @itayd is there a better way to test this works than submitting and creating a draft PR? :-)

@itayd
Copy link
Contributor

itayd commented Jun 22, 2021

unfortunately not yet :( i don't see any warning in this pr though?

@phlax
Copy link
Member

phlax commented Jun 22, 2021

@alyssawilk im a bit confused by this pr - as @itayd said i dont see any warning - and im struggling to understand the connection to repokitteh as this change is to the github notifier

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Copy link
Contributor Author

whoops, I had two branches named draft in 2 different trees and pushed the wrong one.
Given it's minimal code I'll just merge the two - sorry about the force push but it's like 6 lines of overwrite so hopefully not bad =P

@alyssawilk alyssawilk removed their assignment Jun 22, 2021

"""

DRAFT_MESSAGE = """
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should perhaps just include this in the main message

the reason being that until recently i didnt discover how to create a draft pr - so habitually created a non-draft and then marked it so

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm - altho i guess this also sends a message to non-new-contributors - which is kinda confusing given the name of this module

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, if someone has contributed a few times and then does a draft I think they should get the warning. I don't think there is a hook for "has ever done a draft before" so it's all or nothing.
moved the module for clarity

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
phlax
phlax previously approved these changes Jun 23, 2021
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - we'll have to "first song sound check" with it i reckon as no easy way to test repokitteh

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks

@alyssawilk alyssawilk merged commit b68fa25 into envoyproxy:main Jun 23, 2021
chrisxrepo pushed a commit to chrisxrepo/envoy that referenced this pull request Jul 8, 2021
Updating contributing docs as well

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: chris.xin <xinchuantao@qq.com>
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
Updating contributing docs as well

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk deleted the draft branch February 28, 2022 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants